Repository navigation
Conversation
Some translated catalogues contain msgstrs with damaged str.format placeholders. Six call sites formatted translated text eagerly, so a single damaged placeholder in any locale crashed the build with a KeyError. Add sphinx.locale.safe_format, which formats the translation and falls back to the untranslated message when the placeholder sets do not match or formatting raises, and route the eager call sites through it. Refs sphinx-doc#14664
|
Heads-up on the red test matrix: the failing set here ( For what it's worth, the suites covering the code this branch touches pass in CI ( |
|
Quick status update now that #14611 has landed: the branch is updated and the test matrix is green, apart from the queue-wide Docutils HEAD and LaTeX jobs that are still red across the open PR queue. Ready for review whenever someone has a window. |
|
Ping for review when someone has a cycle — this fixes the safe_format placeholder-set fallback discussed in the issue (code half; Transifex half remains separate). Branch is conflict-free against current master. |
m-aciek
left a comment
m-aciek
left a comment
There was a problem hiding this comment.
With those changes python babel_runner.py extract may miss those strings on which we use safe_format. Maybe we can configure babel for a new function name, or fix it in some other way?
Otherwise those changes look good to me, thank you.
Teach the extraction keywords about `safe_format` (its first argument is the msgid) and `_NodeUpdater.compare_references` (its third argument is the untranslated warning message), so that the strings moved behind `safe_format` in sphinx-doc#14664 keep appearing in `sphinx/locale/sphinx.pot`.
|
Thank you for catching this — confirmed, and it's actually worse than "may miss": I ran I went with your "configure babel" suggestion, in 440f111: KEYWORDS = {
**DEFAULT_KEYWORDS,
'_': None,
'__': None,
'safe_format': None,
'compare_references': (3,),
}
Verified by re-running extraction on the branch: the regenerated template contains all nine msgids again, with nothing else lost (861 → 870 msgids, 9 new / 0 lost against the unfixed run). |
|
@m-aciek @picnixz gentle ping on this — the branch is refreshed against current master and the matrix is green, with Docutils HEAD the only red job (it is failing in its own install step on this run). The babel extract concern from the earlier review is addressed and verified in the thread above. Is there anything else you would like changed here, or could this move? |
|
After thinking it through, I think we should change strategy and prevent including incorrect translation catalogs in pull requests with updates from Transifex instead of allowing it and working it around in runtime. I'm sorry for late feedback. |
|
No worries about the timing — prevention at the sync is the cleaner call, and the runtime tolerance was always the workaround half. I will close this PR then; most of it (the safe_format fallback and the call-site changes) only exists to power that workaround. What I can contribute without Transifex access is the gate itself: a check that each catalogue's format placeholders match its msgid — it would have caught all three el/fa cases from the issue — wired into CI for any PR that touches the catalogues, including the sync ones. Happy to send that as a fresh PR if it is welcome. |
|
It looks like the underlying issues were fixed on Transifex about 5 months ago by Jarred Dillard. I think a PR for CI improvement would still be welcome (though please note I'm not a maintainer). I think a good solution would be to check the catalogs when pulling with |
|
PR up as #14784. It turned out babel Catalog.check() reports zero errors on the damaged el/fa entries — it only validates messages flagged python-brace-format, which the Transifex catalogues almost never carry — so the check compares placeholders directly via string.Formatter rather than msgfmt, since the runners do not have gettext 0.23 yet. On current master it flags exactly the four messages from the issue and nothing else across all 71 catalogues. |
Refs #14664.
Problem
As detailed in #14664, some translated catalogues (e.g.
el,fa) contain msgstrs with damagedstr.formatplaceholders:Six call sites format translated text eagerly, so a single damaged placeholder in any locale kills the build with a
KeyErrorthat surfaces as anExtensionErrorblaming an extension (sphinx/config.pyx3,sphinx/_cli/__init__.pyx2,sphinx/transforms/i18n.py).Approach
Add
sphinx.locale.safe_format: pass the untranslated message and the format arguments; it formats the translation and falls back to the original English message when the translation is damaged.Damage detection compares the set of
str.formatfield names between the msgid and its translation, plus a try/except around the format call:{current.__name__}->{current__name__}) and dropped placeholders are both caught. A plain try/except alone cannot catch dropped placeholders, becausestr.formatsilently ignores surplus arguments and would emit an incomplete message.{1} ... {0}) working..format(the untranslated message formats itself).Affected messages degrade to an English warning instead of a dead build. The catalogue data itself (the Transifex half of #14664) is not touched here ΓÇö that needs someone with Transifex access, per the discussion in the issue.
Tests
tests/test_locale.py: unit tests for the helper (healthy translation, renamed keyword, dropped positional, mixed corruption, reordered placeholders, other namespaces).tests/test_config/test_config.py: regression test driving a corrupted catalogue throughcheck_confval_types, asserting the English fallback message.tests/test_intl/test_intl.pysuite passes (61 tests) ΓÇö thecompare_referenceswarnings now format through the helper.AI Disclosure
Per the PR template: the opencode coding agent (GLM) was used in preparing this pull request — to survey the translation catalogues for damaged
str.formatplaceholders, draft thesafe_formatfallback, and write the tests. I reviewed the change and validated it locally (fulltest_intlsuite green).